Skip to content

rust: rename Guard to GuardMut.#520

Merged
wedsonaf merged 1 commit intoRust-for-Linux:rustfrom
wedsonaf:guard-mut
Nov 5, 2021
Merged

rust: rename Guard to GuardMut.#520
wedsonaf merged 1 commit intoRust-for-Linux:rustfrom
wedsonaf:guard-mut

Conversation

@wedsonaf
Copy link
Copy Markdown

This is in preparation for the introduction of Guard, which won't
implement DerefMut. It will be used in sequence locks where the
protected data cannot be directly modified because it can be accessed
concurrently by readers.

This is a pure refactor with no functional changes intended.

Signed-off-by: Wedson Almeida Filho wedsonaf@google.com

This is in preparation for the introduction of `Guard`, which won't
implement `DerefMut`. It will be used in sequence locks where the
protected data cannot be directly modified because it can be accessed
concurrently by readers.

This is a pure refactor with no functional changes intended.

Signed-off-by: Wedson Almeida Filho <wedsonaf@google.com>
@nbdd0121
Copy link
Copy Markdown
Member

Does this mean we'll use Guard, GuardMut instead of ReadGuard, WriteGuard for reader-writer locks (with GuardMut also serve as the guard for exclusive locks)? This is slightly different from other Rust lock designs but I think is reasonable.

@wedsonaf
Copy link
Copy Markdown
Author

Does this mean we'll use Guard, GuardMut instead of ReadGuard, WriteGuard for reader-writer locks (with GuardMut also serve as the guard for exclusive locks)? This is slightly different from other Rust lock designs but I think is reasonable.

This is a different dimension: the guard for the write-side critical section of a sequential lock only exposes immutable references to the underlying data. So if we were to use ReadGuard and WriteGuard, the write method of a lock would be returning a ReadGuard, which isn't very intuitive. I think the mutable vs immutable distinction (as in Iter/IterMut and Cursor/CursorMut) seems more descriptive and we have precedents of their use.

I'm not really a big fan of the Mut suffix though. If you have better ideas, I'd be happy to change.

@nbdd0121
Copy link
Copy Markdown
Member

Seqlocks probably require their own abstraction, e.g. https://docs.rs/seqlock/. (I would probably change the read method to take the signature pub fn read<U>(&self, f: impl FnMut(T) -> U) -> U)

@wedsonaf
Copy link
Copy Markdown
Author

Seqlocks probably require their own abstraction, e.g. https://docs.rs/seqlock/. (I would probably change the read method to take the signature pub fn read<U>(&self, f: impl FnMut(T) -> U) -> U)

The seqlock in the kernel behaves differently, we don't necessarily want (and can't in some cases) copy a whole T out of the lock. Anyway, the read side is definitely different enough that we don't want to try to merge it into the existing abstractions, but the write side is very much conventional -- ideally it should work as a lock for CondVar and LockedBy.

@nbdd0121
Copy link
Copy Markdown
Member

I am not sure if exposing immutable references to seqlock write side is the correct abstraction.

I think it's better a have the better picture of seqlock API design before renaming this.

@fbq
Copy link
Copy Markdown
Member

fbq commented Oct 16, 2021

I am not sure if exposing immutable references to seqlock write side is the correct abstraction.

Unlike rwlock, for seqlock (and RCU), read side and write side are not mutually exclusive, so the uniqueness of the references in the write side cannot be guaranteed.

I think it's better a have the better picture of seqlock API design before renaming this.

I agree. Having some POC code definitely help discussion. @wedsonaf I think you actually have the draft implementation already?

@fbq
Copy link
Copy Markdown
Member

fbq commented Oct 17, 2021

I wonder whether we can add "mutability" to interior mutable type? Something as the following:

pub struct NonRaceUsize {
    val: AtomicUsize
}

impl NonRaceUsize {
    pub fn store(&mut self, v: usize) {
        self.val.store(v, Ordering::Relaxed);
    }
    
    pub fn load(&self) -> Usize{
        self.val.load(Ordering::Relaxed);
    }
}

Will it still be UB if we have (and access) both &mut NonRaceUsize and &NonRaceUsize at the same time?

@nbdd0121
Copy link
Copy Markdown
Member

Unlike rwlock, for seqlock (and RCU), read side and write side are not mutually exclusive, so the uniqueness of the references in the write side cannot be guaranteed.

In LLVM there is an "unordered" ordering that is more relaxed than "relaxed" ordering; it basiscally is a non-atomic access that is defined to be not UB when data race happens. So it's okay if one side is having mutable access and another is only performing "unordered" read access. There is some use of it in compiler builtins but since it's a LLVM thing and not formally defined in Rust so we definitely want to avoid it.

@nbdd0121
Copy link
Copy Markdown
Member

Will it still be UB if we have (and access) both &mut NonRaceUsize and &NonRaceUsize at the same time?

It would still be UB under Rust memory model.

@nbdd0121
Copy link
Copy Markdown
Member

I had this idea: https://godbolt.org/z/63qsTsave

There are tons of scaffolding, but the usage is really just the last few lines. The key idea is to use &mut ZST to represent the permission to the whole struct.

@fbq
Copy link
Copy Markdown
Member

fbq commented Oct 17, 2021

Unlike rwlock, for seqlock (and RCU), read side and write side are not mutually exclusive, so the uniqueness of the references in the write side cannot be guaranteed.

In LLVM there is an "unordered" ordering that is more relaxed than "relaxed" ordering; it basiscally is a non-atomic access that is defined to be not UB when data race happens. So it's okay if one side is having mutable access and another is only performing "unordered" read access. There is some use of it in compiler builtins but since it's a LLVM thing and not formally defined in Rust so we definitely want to avoid it.

Or formally define it? ;-) Rust definitely don't want to rely on a particular feature of the backend, but can ask the backends to provide certain guarantee, right?

The basic idea is that data races are fine as long as the value of a raced read is never "used", seqlock is one example: reads are considered to be correct only when sequence counts are stable (during begin and retry), which means we are racing with a write side critical section (in memory effects).

That said, the proper definition of "used" may be challenging, there are cases in Linux kernel where a value of a raced read is used: 1) for diagnostic code, which doesn't care that much about correctness, 2) fast check for whether a fastpath can be used, e.g. if (raced_val) { fastpath_will_reread_value_with_locks(...); }, etc.

BTW, the implementation of https://docs.rs/seqlock/ also has UB, right?

@nbdd0121
Copy link
Copy Markdown
Member

BTW, the implementation of docs.rs/seqlock also has UB, right?

Technically yes. Practically, it couldn't be exploited by the compiler. The volatile read is essentially used to mimic the unordered load; it'll be UB-free if unordered load is used (at least according to LLVM semantics).

@fbq
Copy link
Copy Markdown
Member

fbq commented Oct 17, 2021

I had this idea: https://godbolt.org/z/63qsTsave

There are tons of scaffolding, but the usage is really just the last few lines. The key idea is to use &mut ZST to represent the permission to the whole struct.

Try to see if I understand your point, it works because current Rust memory model allows the coexistence&mut ZST and &ZST so it's not UB, and since the real accesses are implemented by atomic, therefore no data race, therefore no UB?

@nbdd0121
Copy link
Copy Markdown
Member

it works because current Rust memory model allows the coexistence&mut ZST and &ZST so it's not UB

Yes. Two pointers alias if the memory regions they point to overlap. Since ZST pointers point to a memory region of 0 bytes, if cannot alias with any pointer by definition.

and since the real accesses are implemented by atomic, therefore no data race, therefore no UB?

Yes

@wedsonaf
Copy link
Copy Markdown
Author

I am not sure if exposing immutable references to seqlock write side is the correct abstraction.

Unlike rwlock, for seqlock (and RCU), read side and write side are not mutually exclusive, so the uniqueness of the references in the write side cannot be guaranteed.

I think it's better a have the better picture of seqlock API design before renaming this.

I agree. Having some POC code definitely help discussion. @wedsonaf I think you actually have the draft implementation already?

Here's the commit for the seqlock: f0e3f12

@wedsonaf
Copy link
Copy Markdown
Author

I had this idea: https://godbolt.org/z/63qsTsave

There are tons of scaffolding, but the usage is really just the last few lines. The key idea is to use &mut ZST to represent the permission to the whole struct.

I like the idea of using a ZST, but how is one supposed to use this? Please don't tell me we need a proc macro to parse type definitions and generate all this code.

@wedsonaf
Copy link
Copy Markdown
Author

wedsonaf commented Nov 5, 2021

I opened #537 so that we can investigate other designs for seqlocks. For now I'll submit this one and we can replace it later if we find a better one.

@wedsonaf wedsonaf merged commit 79f97bf into Rust-for-Linux:rust Nov 5, 2021
@wedsonaf wedsonaf deleted the guard-mut branch November 5, 2021 14:18
ojeda pushed a commit that referenced this pull request Aug 25, 2025
If the argument check during an array bind fails, the bind_ops are freed
twice as seen below. Fix this by setting bind_ops to NULL after freeing.

==================================================================
BUG: KASAN: double-free in xe_vm_bind_ioctl+0x1b2/0x21f0 [xe]
Free of addr ffff88813bb9b800 by task xe_vm/14198

CPU: 5 UID: 0 PID: 14198 Comm: xe_vm Not tainted 6.16.0-xe-eudebug-cmanszew+ #520 PREEMPT(full)
Hardware name: Intel Corporation Alder Lake Client Platform/AlderLake-P DDR5 RVP, BIOS ADLPFWI1.R00.2411.A02.2110081023 10/08/2021
Call Trace:
 <TASK>
 dump_stack_lvl+0x82/0xd0
 print_report+0xcb/0x610
 ? __virt_addr_valid+0x19a/0x300
 ? xe_vm_bind_ioctl+0x1b2/0x21f0 [xe]
 kasan_report_invalid_free+0xc8/0xf0
 ? xe_vm_bind_ioctl+0x1b2/0x21f0 [xe]
 ? xe_vm_bind_ioctl+0x1b2/0x21f0 [xe]
 check_slab_allocation+0x102/0x130
 kfree+0x10d/0x440
 ? should_fail_ex+0x57/0x2f0
 ? xe_vm_bind_ioctl+0x1b2/0x21f0 [xe]
 xe_vm_bind_ioctl+0x1b2/0x21f0 [xe]
 ? __pfx_xe_vm_bind_ioctl+0x10/0x10 [xe]
 ? __lock_acquire+0xab9/0x27f0
 ? lock_acquire+0x165/0x300
 ? drm_dev_enter+0x53/0xe0 [drm]
 ? find_held_lock+0x2b/0x80
 ? drm_dev_exit+0x30/0x50 [drm]
 ? drm_ioctl_kernel+0x128/0x1c0 [drm]
 drm_ioctl_kernel+0x128/0x1c0 [drm]
 ? __pfx_xe_vm_bind_ioctl+0x10/0x10 [xe]
 ? find_held_lock+0x2b/0x80
 ? __pfx_drm_ioctl_kernel+0x10/0x10 [drm]
 ? should_fail_ex+0x57/0x2f0
 ? __pfx_xe_vm_bind_ioctl+0x10/0x10 [xe]
 drm_ioctl+0x352/0x620 [drm]
 ? __pfx_drm_ioctl+0x10/0x10 [drm]
 ? __pfx_rpm_resume+0x10/0x10
 ? do_raw_spin_lock+0x11a/0x1b0
 ? find_held_lock+0x2b/0x80
 ? __pm_runtime_resume+0x61/0xc0
 ? rcu_is_watching+0x20/0x50
 ? trace_irq_enable.constprop.0+0xac/0xe0
 xe_drm_ioctl+0x91/0xc0 [xe]
 __x64_sys_ioctl+0xb2/0x100
 ? rcu_is_watching+0x20/0x50
 do_syscall_64+0x68/0x2e0
 entry_SYSCALL_64_after_hwframe+0x76/0x7e
RIP: 0033:0x7fa9acb24ded

Fixes: b43e864 ("drm/xe/uapi: Add DRM_XE_VM_BIND_FLAG_CPU_ADDR_MIRROR")
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Signed-off-by: Christoph Manszewski <christoph.manszewski@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Link: https://lore.kernel.org/r/20250813101231.196632-2-christoph.manszewski@intel.com
(cherry picked from commit a01b704)
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants